Add withKnownIssue comments to known Issues #1014

aroben · 2025-03-11T17:41:24Z

Add withKnownIssue comments to known Issues

Motivation:

The Comment passed to withKnownIssue() is currently only used when a known issue does not occur. When a known issue does occur, it is marked isKnown but does not contain any record of the withKnownIssue() comment, making it harder to understand why the issue was considered to be known, especially if there are multiple nested withKnownIssue() calls.

Modifications:

When an issue is marked "known" by a withKnownIssue() call, the recorded issue will now have a new knownIssueContext property containing the comment passed to withKnownIssue() (if any). This comment will be included in the messages array of the ABI.EncodedEvent that represents the issue.

If the issue is recorded within multiple nested withKnownIssue() calls, knownIssueContext corresponds to the innermost matching call.

The Issue.isKnown setter is now deprecated and a no-op.

When an error is thrown within a withKnownIssue() call, the Issue passed to the issue matcher used to have the known issue comment in Issue.comments. That has now been removed; Issues for thrown errors now have no comments at all when passed to the issue matcher. This matches Issues for thrown errors that occur outside of withKnownIssue() calls.

Checklist:

Code and documentation should follow the style of the Style Guide.
If public symbols are renamed or modified, DocC references should be updated.

aroben · 2025-03-11T18:03:42Z

@swift-ci test

### Motivation: The Comment passed to withKnownIssue() is currently only used when a known issue does not occur. When a known issue does occur, it is marked isKnown but does not contain any record of the withKnownIssue() comment, making it harder to understand why the issue was considered to be known, especially if there are multiple nested withKnownIssue() calls. ### Modifications: When an issue is marked "known" by a `withKnownIssue()` call, the recorded issue will now have multiple comments in its `comments` array: 1. The comment passed to `withKnownIssue()`, if any 2. The Issue's own comment(s) If the issue is recorded within multiple nested `withKnownIssue()` calls, only the comment of the innermost matching call is added to the Issue. ### Checklist: - [x] Code and documentation should follow the style of the [Style Guide](https://github.com/apple/swift-testing/blob/main/Documentation/StyleGuide.md). - [x] If public symbols are renamed or modified, DocC references should be updated.

aroben · 2025-03-11T18:17:34Z

@swift-ci test

aroben · 2025-03-11T19:43:23Z

@swift-ci test

aroben · 2025-03-11T20:02:07Z

@swift-ci test

Sources/Testing/Issues/Issue+Recording.swift

aroben · 2025-03-12T14:06:39Z

@stmontgomery I took a crack at putting the known-issue data into a separate stored property and converting isKnown to a computed property. Some questions:

I left a setter in place for isKnown to try to preserve ABI compatibility. Does this seem important to you?
What do you think about the names Issue.knownIssueContext and Issue.KnownIssueContext?
I replaced EncodedIssue.isKnown with a new EncodedIssue.knownIssueContext and made KnownIssueContext conform to Codable. Does that seem like the right approach? Do I need to bump the ABI version or do anything else to handle compatibility?

aroben · 2025-03-12T14:06:55Z

@swift-ci test

aroben · 2025-03-12T14:10:59Z

@swift-ci test

aroben · 2025-03-12T14:18:33Z

@swift-ci test

aroben · 2025-04-07T14:19:38Z

After discussion with @grynspan and @stmontgomery I've reverted the ABI changes. Instead, we append the known issue comment (if any) to ABI.EncodedEvent.messages for the .issueRecorded event.

aroben · 2025-04-07T14:27:07Z

@swift-ci please test

aroben · 2025-04-07T14:54:29Z

I wanted to see if the known issue comment now appears in swift test's output, so I added a new suite to this repo:

struct TryIt {
  @Test func knownIssueTest() async throws {
    withKnownIssue("known issue comment") {
      Issue.record("issue comment")
    }
  }

  @Test func knownIssueTest_throws() async throws {
    withKnownIssue("known issue comment") {
      struct TheError: Error {}
      throw TheError()
    }
  }
}

Here's what I got:

$ SWT_SF_SYMBOLS_ENABLED=no xcrun swift test --filter TryIt      
Building for debugging...
[1/1] Write swift-version--1EBDF3DDE0C0C582.txt
Build complete! (0.16s)
Test Suite 'Selected tests' started at 2025-04-07 10:48:10.677.
Test Suite 'swift-testingPackageTests.xctest' started at 2025-04-07 10:48:10.678.
Test Suite 'swift-testingPackageTests.xctest' passed at 2025-04-07 10:48:10.678.
	 Executed 0 tests, with 0 failures (0 unexpected) in 0.000 (0.000) seconds
Test Suite 'Selected tests' passed at 2025-04-07 10:48:10.678.
	 Executed 0 tests, with 0 failures (0 unexpected) in 0.000 (0.001) seconds
◇ Test run started.
↳ Testing Library Version: feb96cf762e5732f8702e1dc9755fa2908bfb5ba (modified)
◇ Suite TryIt started.
◇ Test knownIssueTest_throws() started.
◇ Test knownIssueTest() started.
✘ Test knownIssueTest() recorded a known issue at EventRecorderTests.swift:660:19: Issue recorded
↳ issue comment
↳ known issue comment
✘ Test knownIssueTest() passed after 0.001 seconds with 1 known issue.
✘ Test knownIssueTest_throws() recorded a known issue at EventRecorderTests.swift:665:19: Caught error: TheError()
↳ known issue comment
↳ known issue comment
✘ Test knownIssueTest_throws() passed after 0.001 seconds with 1 known issue.
✘ Suite TryIt passed after 0.001 seconds with 2 known issues.
✘ Test run with 2 tests passed after 0.001 seconds with 2 known issues.

This looks decent, but you can see that known issue comment is duplicated for knownIssueTest_throws(). This is because _matchError puts the known issue comment directly in the Issue.comments array, but then we also include it in Issue.knownIssueContext, so we end up with two copies of the comment.

Here are some ways to handle this:

Leave it as-is
Remove the known issue comment from Issue.comments, leaving it only in Issue.knownIssueContext.
a. This seems right from a modeling perspective, but will in practice remove the known issue comment from Xcode's inline error annotation UI
Omit the known issue comment from Issue.knownIssueContext for thrown errors
a. This feels bad, though I'm not sure it has any practical downsides.
Teach HumanReadableOutputRecorder to somehow detect this case and avoid the duplication
a. We should be able to detect this reliably: if case .errorCaught = issue.kind, issue.isKnown

I'm leaning toward 4 (and would add a test for it of course).

aroben · 2025-04-07T15:28:56Z

I'm leaning toward 4 (and would add a test for it of course).

I implemented this.

aroben · 2025-04-07T15:29:00Z

@swift-ci please test

Tests/TestingTests/EventRecorderTests.swift

Sources/Testing/Events/Recorder/Event.HumanReadableOutputRecorder.swift

Co-authored-by: Jonathan Grynspan <[email protected]>

aroben · 2025-04-07T16:24:59Z

@swift-ci please test

stmontgomery

A few nitpicky formatting comments I saw as I caught up with the latest changes

Sources/Testing/Events/Recorder/Event.HumanReadableOutputRecorder.swift

Sources/Testing/Issues/KnownIssue.swift

Sources/Testing/Events/Recorder/Event.HumanReadableOutputRecorder.swift

Co-authored-by: Stuart Montgomery <[email protected]>

aroben · 2025-04-24T14:18:08Z

@swift-ci please test

Sources/Testing/Issues/KnownIssue.swift

stmontgomery · 2025-04-24T15:33:38Z

Tests/TestingTests/KnownIssueTests.swift

Great tests!

stmontgomery

Just a few more suggestions and style cleanups, but I think the behavior and logic all make sense now!

Co-authored-by: Stuart Montgomery <[email protected]>

aroben · 2025-04-24T19:25:22Z

@swift-ci please test

stmontgomery · 2025-04-24T19:41:37Z

The macOS and Linux CI failures are expected to be resolved by swiftlang/swift#80830, but the release of a new main snapshot toolchain is currently blocked for unrelated reasons.

aroben requested review from stmontgomery, grynspan, briancroom, SeanROlszewski and suzannaratcliff as code owners March 11, 2025 17:41

grynspan assigned aroben Mar 11, 2025

grynspan added bug 🪲 Something isn't working issue-handling Related to Issue handling within the testing library labels Mar 11, 2025

grynspan added this to the Swift 6.2 milestone Mar 11, 2025

aroben force-pushed the known-issue-comments branch from cf99d11 to c6cbd3d Compare March 11, 2025 18:17

stmontgomery reviewed Mar 11, 2025

View reviewed changes

Sources/Testing/Issues/Issue+Recording.swift Outdated Show resolved Hide resolved

stmontgomery reviewed Mar 11, 2025

View reviewed changes

Sources/Testing/Issues/Issue+Recording.swift Outdated Show resolved Hide resolved

aroben added 3 commits March 12, 2025 09:42

KnownIssueContext.current

acdb283

KnownIssueContext -> KnownIssueScope

08b1c3f

Issue.knownIssueContext

45f8d9f

aroben added 4 commits March 12, 2025 10:08

Update comment

e2da439

Fix typo

e0251d8

Remove KnownIssueScope.Match

89b0a67

Renames for clarity

deffa29

Update tests

e6e4fd4

aroben requested a review from stmontgomery March 12, 2025 18:01

Rename KnownIssueScope.match -> matcher

feb96cf

Deduplicate known issue comments for thrown errors

874f830

grynspan reviewed Apr 7, 2025

View reviewed changes

Tests/TestingTests/EventRecorderTests.swift Outdated Show resolved Hide resolved

grynspan reviewed Apr 7, 2025

View reviewed changes

Sources/Testing/Events/Recorder/Event.HumanReadableOutputRecorder.swift Outdated Show resolved Hide resolved

aroben and others added 2 commits April 7, 2025 12:21

Use Locked.rawValue

6caad07

Co-authored-by: Jonathan Grynspan <[email protected]>

Explain why we treat .errorCaught specially

316d10c

stmontgomery reviewed Apr 9, 2025

View reviewed changes

aroben and others added 2 commits April 24, 2025 10:00

Apply suggestions from code review

8567a61

Co-authored-by: Stuart Montgomery <[email protected]>

Don't put the known issue comment in Issue.comments for thrown errors

1ae8633